Skip to content

Common tests #2474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Common tests #2474

wants to merge 11 commits into from

Conversation

mrginglymus
Copy link
Contributor

@mrginglymus mrginglymus commented Dec 28, 2024

Adding a new test was a bit of a PITA due to the sheer number of libraries involved.

This proposed change (with example implementations in riot and react) moves all test definitions to a common location, with frameworks only having to invoke them with a set of factory functions.

This should hopefully make it easier to:

  1. Add a new test across all libraries (libraries missing implementations will gracefully skip)
  2. Add a new library (tests missing implementations will gracefully skip)
  3. Critically, ensure consistency of behaviour in tests

I can't see what skips will look like until #2475 is merged :)

I may pick off a few others to test this works, but I wont commit to completing until it's approved in principle.

@mrginglymus mrginglymus marked this pull request as ready for review December 28, 2024 22:04
@Westbrook
Copy link
Collaborator

Slowly making the way through all of your contributions here @mrginglymus. This one is a little meatier, so I can't promise I get through it very quickly, but if you'll catch up to main, I'll do my best. 🙇🏼

@mrginglymus
Copy link
Contributor Author

Amazing, thanks @Westbrook! All up to date now. One outstanding bit of work on this is checking how 'skipped' appears on the output docs - we obviously don't have nay currently skipped tests.

@mrginglymus
Copy link
Contributor Author

Created a branch (#2513) to demonstrate a skipped test.

This shows effectively as a fail (or a 'not pass') on the front page:

image

and a skip on details:

image

@mrginglymus
Copy link
Contributor Author

Do we want to count skipped tests as fails on this headline (show 14/16) or do we want to not count them at all (show 14/14, 100%). The first option doesn't seem particularly fair, the second isn't particularly accurate....

@Westbrook
Copy link
Collaborator

There aren't currently skipped tests, right? Do you foresee expanded to skipped tests in the near future?

@mrginglymus
Copy link
Contributor Author

Skipped tests will happen when a new test is added that requires a new component. Until that component is implemented in each framework, it will show as skipped.

The benefit of this PR is that adding or modifying a test will immediately be reflected in all frameworks.

The downside is that adding a test that requires a new component requires that component to be implemented in each framework.

Making unimplemented components skip tests I felt was a reasonable compromise to allow us to continue to add new tests without being fully blocked by the tedium of having to implement every framework.

Obviously we should aim for complete coverage, but at least for local development having tests skip instead of fail will make the process smoother

@mrginglymus
Copy link
Contributor Author

I've just tried using this to add an aria-selected={false} test and...it's a right pain having to update all of the values. I don't think that skipped tests should be included in the score or in e.g.

  "basicSupport": {
    "total": 20,
    "failed": 4,
    "passed": 16
  },

otherwise one has to update all the files (which is tedious when considering scores) when adding a test.

@mrginglymus
Copy link
Contributor Author

See mrginglymus#1 for an example new + partially implemented test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants